CNTRLPLANE-3361: Align with latest beta API of Vault#2936
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-3361 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThe Vault KMS API replaces Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hello @ardaguclu! Some important instructions when contributing to openshift/api: |
|
/hold |
openshift-ci
Bot
added
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml`:
- Around line 365-366: Extend the vaultKeyPath invalid-test fixtures near
“Should reject vaultKeyPath without /keys/ separator” with cases for a
1025-character mount and a 513-character key, ensuring each complete path
remains under 1542 characters and is rejected by the schema.
In `@config/v1/types_kmsencryption.go`:
- Around line 212-219: Preserve independent mount and key length limits for the
vaultKeyPath validation. In config/v1/types_kmsencryption.go lines 212-219, add
CEL checks limiting the mount component to 1024 characters and the key name
component to 512 characters; regenerate the corresponding validation in
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
lines 379-407,
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
lines 379-407, and
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
lines 379-407; add rejection fixtures for independently overlong components in
config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml lines 365-366.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: de2ae300-c773-4f55-9ac3-09c5a6e2950e
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (6)
config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yamlconfig/v1/tests/apiservers.config.openshift.io/VaultKMS.yamlconfig/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
| # vaultKeyPath invalid tests | ||
| - name: Should reject vaultKeyPath without /keys/ separator |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add component-length rejection cases.
Add fixtures for a 1025-character mount and a 513-character key whose complete paths remain below 1542 characters. They currently demonstrate that the schema admits values rejected by the former component contracts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml` around lines
365 - 366, Extend the vaultKeyPath invalid-test fixtures near “Should reject
vaultKeyPath without /keys/ separator” with cases for a 1025-character mount and
a 513-character key, ensuring each complete path remains under 1542 characters
and is rejected by the schema.
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=1542 | ||
| // +kubebuilder:validation:XValidation:rule="!self.startsWith('/')",message="vaultKeyPath cannot start with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="vaultKeyPath cannot end with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains('//')",message="vaultKeyPath cannot contain consecutive forward slashes" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9._~/-]+$')",message="vaultKeyPath must only contain RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward slashes" | ||
| // +kubebuilder:validation:XValidation:rule="self.split('/').filter(s, s == '.' || s == '..').size() == 0",message="vaultKeyPath must not contain '.' or '..' path segments" | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-zA-Z0-9._~-]+(/[a-zA-Z0-9._~-]+)*/keys/[a-zA-Z0-9_]([a-zA-Z0-9_.-]*[a-zA-Z0-9_])?$')`,message="vaultKeyPath must follow the format <mount>/keys/<key-name> where the key name starts and ends with an alphanumeric character or underscore and may contain alphanumeric characters, underscores, hyphens, and periods" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the independent mount and key length limits. The aggregate 1542-character limit permits either component to exceed its previous contract.
config/v1/types_kmsencryption.go#L212-L219: add CEL validation limiting the mount to 1024 and key name to 512 characters.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml#L379-L407: regenerate the corrected validation.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml#L379-L407: regenerate the corrected validation.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml#L379-L407: regenerate the corrected validation.config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml#L365-L366: add rejection fixtures for independently overlong components.
📍 Affects 5 files
config/v1/types_kmsencryption.go#L212-L219(this comment)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml#L379-L407payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml#L379-L407payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml#L379-L407config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml#L365-L366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1/types_kmsencryption.go` around lines 212 - 219, Preserve
independent mount and key length limits for the vaultKeyPath validation. In
config/v1/types_kmsencryption.go lines 212-219, add CEL checks limiting the
mount component to 1024 characters and the key name component to 512 characters;
regenerate the corresponding validation in
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
lines 379-407,
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
lines 379-407, and
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
lines 379-407; add rejection fixtures for independently overlong components in
config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml lines 365-366.
Source: Path instructions
|
/test integration |
| // +required | ||
| VaultKeyPath string `json:"vaultKeyPath,omitempty"` | ||
|
|
||
| // --- TOMBSTONE --- |
There was a problem hiding this comment.
do we really need a tombstone value here?
the API was introduced in this release and the entire feature is behind a GA feature gate. I’m sure there are no clusters where this struct would already be populated.
There was a problem hiding this comment.
I agree with you and I would be happy to remove those fields without tombstoning. But I think, tombstoning will be asked by API reviewers cc: @JoelSpeed @everettraven
There was a problem hiding this comment.
I think, this comment #2931 (comment) also applies here.
| TransitMount string `json:"transitMount,omitempty"` | ||
| // TransitMount string `json:"transitMount,omitempty"` | ||
|
|
||
| // --- TOMBSTONE --- |
|
verify-crd-schema, verify-crdify are expected. verify-hypershift-integration seems to be perma-failing on every PR. |
|
@ardaguclu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Vault notifies us about their planned API changes in the new beta image. This new version brings some breaking changes. If we were in GA, we must have made changes in backwards compatible way. However, we haven't promoted this feature to GA yet. That's why, this PR tombstones the deprecated fields (i.e. transit-mount, transit-key), introduces one required field
vault-key-pathand one optional fieldvault-auth-namespace.